Swift: Add tests and develop command injection query #13906

geoffw0 · 2023-08-07T13:16:11Z

Add tests and develop the command injection query (swift/command-line-injection). I haven't promoted it out of experimental yet, as there are a few more things I still hope to fix when we have all the data flow content work merged.

Currently there's also regression on MRVA (1 result, in vineetchoudhary/Downloader-for-Apple-Developers), a result I believe to be a true positive that the query isn't finding in it's current form. I believe this may be related to the MRVA snapshot for that project being old, i.e. an extractor bug that was fixed recently. So I expect that will fix itself, and if it doesn't I'll address the problem in a follow-up pull request as well.

@maikypedia FYI

github-actions · 2023-08-07T13:17:24Z

QHelp previews:

swift/ql/src/experimental/Security/CWE-078/CommandInjection.qhelp

System command built from user-controlled sources

Constructing a system command with unsanitized user input is dangerous, since a malicious user may be able to craft input that executes arbitrary code.

Recommendation

If possible, use hard-coded string literals to specify the command to run. Instead of interpreting user input directly as command names, examine the input and then choose among hard-coded string literals.

If this is not possible, then add sanitization code to verify that the user input is safe before using it.

Example

The following example executes code from user input without sanitizing it first:

var task = Process()
task.launchPath = "/bin/bash"
task.arguments = ["-c", userControlledString] // BAD

task.launch()

If user input is used to construct a command it should be checked first. This ensures that the user cannot insert characters that have special meanings:

func validateCommand(_ command: String) -> String? {
    let allowedCommands = ["ls -l", "pwd", "echo"]
    if allowedCommands.contains(command) {
        return command
    }
    return nil
}

if let validatedString = validateCommand(userControlledString) {
    var task = Process()
    task.launchPath = "/bin/bash"
    task.arguments = ["-c", validatedString] // GOOD

    task.launch()
}

References

OWASP: Command Injection.
Common Weakness Enumeration: CWE-78.
Common Weakness Enumeration: CWE-88.

… from the SetContent PR, this fixes the test failures we were getting).

geoffw0 · 2023-08-08T09:59:45Z

@d10c would you mind reviewing this? It consists of improvements to the experimental swift/command-line-injection query that was contributed by a user a few weeks ago (though I haven't quite got to the point of promoting it out of experimental yet). There are also some incidental minor improvements to the URL model in this PR (adding .OptionalSome and .CollectionElement content flow tags to indicate when tainted data is inside an Optional or Set).

d10c

LGTM in general, left some comments.

d10c · 2023-08-10T12:19:23Z

swift/ql/src/experimental/Security/CWE-078/CommandInjectionGood.swift

-var task = Process()
-task.launchPath = "/bin/bash"
-task.arguments = ["-c", validateCommand(userControlledString)] // GOOD
+if let validatedString = validateCommand(userControlledString) {


How would the query know that validatedString, to which userControlledString flows, is not tainted? I see in the test cases that this specific pattern is currently a false positive. Any ideas what it would take to fix it?

I think implementing a barrier for the qualifier of Sequence.contains would do it, the assumption being that such a check is likely to be used to validate the data as in the example above.

I'd like this work to cover all of the injection queries rather than being done ad-hoc.

d10c · 2023-08-10T13:59:48Z

swift/ql/test/query-tests/Security/CWE-079/UnsafeWebViewFetch.expected

@@ -52,23 +44,15 @@ edges
 | UnsafeWebViewFetch.swift:164:21:164:35 | call to getRemoteData() | UnsafeWebViewFetch.swift:186:25:186:25 | remoteString |
 | UnsafeWebViewFetch.swift:164:21:164:35 | call to getRemoteData() | UnsafeWebViewFetch.swift:188:25:188:25 | remoteString |
 | UnsafeWebViewFetch.swift:164:21:164:35 | call to getRemoteData() | UnsafeWebViewFetch.swift:197:24:197:37 | .utf8 |
-| UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) | UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) [some:0] |
-| UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) | UnsafeWebViewFetch.swift:179:52:179:52 | remoteURL |
-| UnsafeWebViewFetch.swift:178:18:178:42 | call to URL.init(string:) | UnsafeWebViewFetch.swift:185:47:185:56 | ...! |


These old non-[some:0] taint steps were unnecessary, right?

Yes I think so. The lines in question (178, 179) are:

let remoteURL = URL(string: remoteString) let remoteURL2 = URL(string: "/path", relativeTo: remoteURL)

The initializer URL.init?(string:) returns an optional, so taint is rightly in the [some:0] enum field of the return value. The old steps were less than ideal.

Notably, nothing in the #select of this test is affected.

d10c · 2023-08-10T14:05:53Z

swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll

@@ -10,6 +10,6 @@ private import codeql.swift.dataflow.ExternalFlow
 */
 private class NsUrlSummaries extends SummaryModelCsv {
  override predicate row(string row) {
-    row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint"
+    row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint"


Interesting, didn't know you could have .OptionalSome in the CSV. Do you know how this string label "OptionalSome" is related to the actual implementation of the [some:0] ContentSet?

Yes, there's a special case in parseContent for OptionalSome, it's just syntactic sugar for EnumElement[some:0] (only cleaner and hopefully less error prone).

d10c · 2023-08-10T14:07:27Z

swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll

@@ -83,3 +48,24 @@ private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node
 private class DefaultCommandInjectionSink extends CommandInjectionSink {
  DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }


Interesting pattern: specify domain-specific sinks/sources/summaries as CSV with a custom label.

We do this for sinks in most queries now. In some cases (such as this one) we define the intended sinks via CSV, in other cases they're defined in QL but a CSV sink like this is provided purely as an extension point for users.

Note that "command-injection" is the same string as is used in cs/command-line-injection and java/command-line-injection. There's been an [imperfect] effort to unify these labels across languages where we can.

d10c

👍🏻

geoffw0 added 9 commits August 4, 2023 17:01

Swift: QLDoc comments and metadata.

048daa9

Swift: Qhelp / examples.

2664c30

Swift: Add tests.

8c2140b

Swift: Unify the two sink models into one (needs further polish).

7b9b96d

Swift: Model sinks explicitly with models-as-data.

1c7d63a

Swift: Use enum content in URL and NSURL models.

348c45d

Swift: Add a test with NSUserScriptTask.

7177189

Swift: Model NSUserScriptTask sinks.

416b731

Swift: Add a few more test cases.

4c8accd

geoffw0 added no-change-note-required This PR does not need a change note Swift labels Aug 7, 2023

geoffw0 requested a review from a team as a code owner August 7, 2023 13:16

github-actions bot added the documentation label Aug 7, 2023

geoffw0 added 3 commits August 7, 2023 19:37

Merge branch 'main' into commandinject2

da34da7

Swift: Accept test failures.

a73354d

Swift: Add more missing details to the URL model (along with a change…

b2d3d46

… from the SetContent PR, this fixes the test failures we were getting).

d10c reviewed Aug 10, 2023

View reviewed changes

d10c approved these changes Sep 12, 2023

View reviewed changes

geoffw0 merged commit 3bf0d66 into github:main Sep 13, 2023

geoffw0 mentioned this pull request Nov 6, 2023

Swift: Promote the command injection query out of experimental #14701

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift: Add tests and develop command injection query #13906

Swift: Add tests and develop command injection query #13906

geoffw0 commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

System command built from user-controlled sources

Recommendation

Example

References

geoffw0 commented Aug 8, 2023

d10c left a comment

d10c Aug 10, 2023

geoffw0 Aug 14, 2023

d10c Aug 10, 2023

geoffw0 Aug 14, 2023

d10c Aug 10, 2023

geoffw0 Aug 14, 2023

d10c Aug 10, 2023

geoffw0 Aug 14, 2023

d10c left a comment

		@@ -83,3 +48,24 @@ private class ProcessSink extends CommandInjectionSink instanceof DataFlow::Node
		private class DefaultCommandInjectionSink extends CommandInjectionSink {
		DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }

Swift: Add tests and develop command injection query #13906

Swift: Add tests and develop command injection query #13906

Conversation

geoffw0 commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

System command built from user-controlled sources

Recommendation

Example

References

geoffw0 commented Aug 8, 2023

d10c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d10c left a comment

Choose a reason for hiding this comment